Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove passing exception as args to super in APIError #1477

Merged
merged 4 commits into from
Jul 5, 2024
Merged

Remove passing exception as args to super in APIError #1477

merged 4 commits into from
Jul 5, 2024

Conversation

mike-flowers-airbnb
Copy link
Contributor

First, thanks for this tool!

We have a pipeline where exceptions from packages such as gspread are pickled and stored for further processing. Unfortunately, the APIError exception is not able to be unpickled as the args it passed to the Exception super are different from what is passed into the exception itself.

The sample test extension exposes this error as seen below:

    def __init__(self, response: Response):
        super().__init__(self._extract_error(response))
        self.response: Response = response
>       self.error: Mapping[str, Any] = response.json()["error"]
E       AttributeError: 'dict' object has no attribute 'json'

gspread/exceptions.py:45: AttributeError

This PR ultimately only replaces the extracted error with the raw response passed to the kwargs. Given that __str__ and __repr__ are overloaded and all other usage of this exception use the other members, there is no visible impact from this change.

@alifeee
Copy link
Collaborator

alifeee commented Jun 8, 2024

The lint is failing for unrelated reasons. Please bear with us while #1478 is solved :)

@lavigne958
Copy link
Collaborator

Hi @mike-flowers-airbnb thank you for this improvement!

I need to run a couple of tests first.

As you remove the use of the function extract_error and we already extract it in the response object just below could you please remove the method extract_error please ?

@mike-flowers-airbnb
Copy link
Contributor Author

Hi @mike-flowers-airbnb thank you for this improvement!

I need to run a couple of tests first.

As you remove the use of the function extract_error and we already extract it in the response object just below could you please remove the method extract_error please ?

Sure thing, removed the obsolete function

@lavigne958
Copy link
Collaborator

@mike-flowers-airbnb problem should be solved now, please rebase your branch on laster commit on master branch in order to pass the CI.

@mike-flowers-airbnb
Copy link
Contributor Author

✅ Done , thanks!

@mike-flowers-airbnb
Remove obsolete function
0469541 
Remove obsolete function and typing annotations
lavigne958
lavigne958 requested changes Jun 16, 2024
View reviewed changes
Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi I ran som regression tests. I found an issue in the current design:

  • current a user is allowed to do something like this:
try:
    sheet = file.del_worksheet_by_id(112341462553536987)
except gspread.exceptions.APIError as e:
    pprint(e.args[0]["status"])

In our current code base, this will return:

  • the first argument passed to the Exception parent class
  • this argument is a dict, so it will look for the key code in this dict.
  • This returns the error status returned by the API.

With your current implementation we face the following error with the above code:

Traceback (most recent call last):
  File "/..../exception_test.py", line 10, in <module>
    print(e.args[0]["code"])
TypeError: 'Response' object is not subscriptable

This is because the first argument passed to the Exception parent class is no longer a dict but a Response object.

I tried the below code that would allow us to make the changes and keep the old behavior and prevent regression.

class APIError(GSpreadException):
    """Errors coming from the API itself,
    such as when we attempt to retrieve things that don't exist."""

    def __init__(self, response: Response):
        super().__init__(response)
        self.response: Response = response
        self.error: Mapping[str, Any] = response.json()["error"]
        self.code: int = self.error["code"]

    @property
    def args(self):
        return [dict(self.error)]

the above @property on args will allow us to catch anyone trying to access the args attribute and return the error object as a dict.

@alifeee what do you think of this workaround ?

apart from that, which could introduce a breaking change, I am fine with the changes.

@mike-flowers-airbnb
Copy link
Contributor Author

Appreciate the callout here @lavigne958 . Definitely don't want to be breaking exception catch flows.

I did some searching and found that there is actually a proper form to expose the ability for pickling to work when args don't match. I had incorrectly assumed that args were supposed to match the underlying Exception args, but it looks like there is a built-in capability to support this scenario by using reduce. If there's no objections, I'd propose that we use this instead as it will achieve the desired effect while not tampering with other lower-level functionality. Let me know what you think.

@lavigne958
Copy link
Collaborator

Appreciate the callout here @lavigne958 . Definitely don't want to be breaking exception catch flows.

I did some searching and found that there is actually a proper form to expose the ability for pickling to work when args don't match. I had incorrectly assumed that args were supposed to match the underlying Exception args, but it looks like there is a built-in capability to support this scenario by using reduce. If there's no objections, I'd propose that we use this instead as it will achieve the desired effect while not tampering with other lower-level functionality. Let me know what you think.

that's a great news, if I understand correctly your point, you don't need these changes anymore from gspread right ?
If yes, then I'll be keen on making the changes your propose, but in the next major release so we can introduce the breaking change with a proper warning and users can adjust themselves.

let me know if that's ok with you and I'll put your issue in the next major release milestone.

@mike-flowers-airbnb
Copy link
Contributor Author

Sorry I mean that I can change this PR so that the exception implements __reduce__ and no compatibility will be lost while also introducing new pickling support. Let me add this in real quick to show.

@mike-flowers-airbnb
Copy link
Contributor Author

mike-flowers-airbnb commented Jun 17, 2024 •
edited
Loading

I just pushed a new commit to implement it. You'll notice that the args property is now completely backwards compatible and the exception is able to be properly pickled/unpickled. I also extended the existing test to include your regression test. Let me know your thoughts, thanks.

@lavigne958
Copy link
Collaborator

I just pushed a new commit to implement it. You'll notice that the args property is now completely backwards compatible and the exception is able to be properly pickled/unpickled. I also extended the existing test to include your regression test. Let me know your thoughts, thanks.

thank you for the prompt reply, I'll have a look and test it. looks qiet good to me !

@mike-flowers-airbnb
Copy link
Contributor Author

@lavigne958 , hey there, just checking in to see if anything else is needed for this PR. Thanks.

@lavigne958
Copy link
Collaborator

@lavigne958 , hey there, just checking in to see if anything else is needed for this PR. Thanks.

Hi so far what is mostly needed is "time", I didn't have any time recently to take a look.

With the coming days I will have more time to take a look at it.

Sorry for the delay.

@mike-flowers-airbnb
Copy link
Contributor Author

@lavigne958 , hey there, just checking in to see if anything else is needed for this PR. Thanks.

Hi so far what is mostly needed is "time", I didn't have any time recently to take a look.

With the coming days I will have more time to take a look at it.

Sorry for the delay.

No worries, definitely understand, thanks for the update!

lavigne958
lavigne958 reviewed Jul 3, 2024
View reviewed changes
Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, with regression test as extra ! thank you.
Only a small detail about the indentation just in case then: looks good to merge

tests/worksheet_test.py Show resolved Hide resolved
lavigne958
lavigne958 approved these changes Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lavigne958 lavigne958 merged commit fc84ef4 into burnash:master Jul 5, 2024
5 checks passed
@alifeee alifeee mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavigne958 lavigne958 lavigne958 approved these changes

@alifeee alifeee alifeee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mike-flowers-airbnb @alifeee @lavigne958